Update llvm-libunwind to v18 #102231
Update llvm-libunwind to v18 #102231
Conversation
dotnet-issue-labeler
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
am11
added
area-NativeAOT-coreclr
and removed
needs-area-label
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
am11
force-pushed
the
feature/vendor/llvm-libunwind-18
branch
from
96ca45b
to
74033b9
Compare
| if(CLR_CMAKE_TARGET_APPLE) | ||
| set(LLVM_LIBUNWIND_SOURCES_BASE | ||
| ${LLVM_LIBUNWIND_SOURCES_BASE} | ||
| src/Unwind_AppleExtras.cpp |
There was a problem hiding this comment.
It was deleted in llvm/llvm-project@1ae57fe.
|
If these patches are getting difficult to update, are there subsets we can upstream to make it easier/slim down the diff? |
That would be nice; #72655. 😅 |
|
I think this change could use running additional legs for AOT testing. |
| ldp x18,x19, [x0, #0x090] | ||
| ldp x20,x21, [x0, #0x0A0] | ||
| ldp x22,x23, [x0, #0x0B0] | ||
| ldp x24,x25, [x0, #0x0C0] | ||
| ldp x26,x27, [x0, #0x0D0] | ||
| ldp x28,x29, [x0, #0x0E0] | ||
| ldr x30, [x0, #0x100] // restore pc into lr | ||
| ldr x1, [x0, #0x0F8] |
There was a problem hiding this comment.
Just a note to fix it in a separate PR.
This change is actually problematic. Loading SP early may cause corruption of the source context in case it is located on stack and an async signal fires. See #101709. We will need to do something similar to how it was fixed in the linked issue if we need to restore x16 too.
There was a problem hiding this comment.
@janvorli, thanks. In case you missed it, it's the first commit which put v18 code, then second commit applied our patches which undid this change. So in the current state of the PR, ti's not available. Would be nice to sync it with upstream at some point because they touched nearby code in recent updates.
There was a problem hiding this comment.
I understand, I meant that our state was problematic and that the V18 way is correct, except that it trashes x16. But I have missed the comment above that says that trashing x16 is ok. So maybe we should take the new code here?
There was a problem hiding this comment.
Sure, we can try it in a follow-up PR. Seems like it was like that from the beginning of the port: https://github.com/dotnet/corert/blame/c6af4cfc8b625851b91823d9be746c4f7abdc667/src/Native/libunwind/src/UnwindRegistersRestore.S#L585 and we diverged from upstream in llvm 9 timeframe llvm/llvm-project@d5323f6 (a change we never picked).
There was a problem hiding this comment.
Right, I guess we have just missed that change and skipping it was not intentional.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Timeouts look unrelated. Perhaps one more outerloop run will help validating the update? 👀 |
|
I can see that Threading NativeAOT tests are failing, which was fixed last week. Let me close and reopen the PR to pick the fix to this issue and rerun the outerloop. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
In the history tabs of those failing tests in AzDo, they seem to have intermittently failed before on main as well, so I'm not completely sure if they are related to this version bump. |
|
linux-arm failure is being fixed by #102760 and System.IO tests on linux-x64 are also failing on main. e..g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=689281&view=logs&j=20e6f673-0b0c-5c85-377c-5d941d4c8ee3&t=5801e1cc-59c5-5e47-8484-142b970afd58 Opened #102800 for linux-x64. |
* Update llvm-libunwind to v18 * Apply all patches * Update version file
First commit: deleted
llvm-libunwind/and downloaded fresh copy from v18.1.5 tree.Second commit: apply all our local patches.
Third commit: updated the version file (noting second commit).
These local patches are growing, making it a bit tricky to update.